PR Review: Background sessions on Mongo control plane

Tasks

• Gather context (CLAUDE.md, REVIEW.md)
• Read git diff and key changed files
• Review session persistence / lease logic
• Review session manager changes
• Review routes / main.py changes
• Review worker entrypoint and start.sh
• Review test coverage
• Post findings

View job run

PR Review: Background sessions on Mongo control plane

Tasks

• Gather context (CLAUDE.md, REVIEW.md)
• Read git diff and key changed files
• Review session persistence / lease logic
• Review session manager changes
• Review routes / main.py changes
• Review worker entrypoint and start.sh
• Review test coverage
• Post findings

View job run

Claude finished @akseljoonas's task in 6m 36s —— View job

PR Review: Background sessions on Mongo control plane

Tasks

• Gather context (CLAUDE.md, REVIEW.md)
• Read git diff and key changed files
• Review session persistence / lease logic
• Review session manager changes
• Review routes / main.py changes
• Review worker entrypoint and start.sh
• Review test coverage
• Post findings

2 P0, 3 P1
Verdict: changes requested

What I checked

• Lease CAS logic: claim_lease / renew_lease / release_lease in session_persistence.py
• Full reconnect path from _check_session_access → ensure_session_loaded → _sse_response
• _on_lease_lost scope and which submissions requeue_claimed_for touches
• Heartbeat error handling (renew_lease on PyMongoError)
• SSE holder/non-holder branching in _sse_response and whether the non-holder path is reachable via HTTP
• Worker claim tick: _worker_claim_tick query and session discovery
• Lifespan shutdown sweep: active-turn handover to Workers
• shutdown_session lock scope with external I/O inside
• New /background route auth guard
• _backfill_lease_state migration safety
• Unit test coverage (read test stubs for test_sse_holder_overlay.py, test_session_manager_lease.py)

P0 — Reconnect to Worker-held session returns 404 (acceptance drill 2 is broken)

ensure_session_loaded requires claiming the lease before returning a session object. When a Worker holds the session's lease, claim_lease returns None (filter lease.expires_at < now OR holder_id IS NULL doesn't match a fresh Worker-held lease). ensure_session_loaded returns None at session_manager.py:843–847, and _check_session_access raises HTTP 404 at routes/agent.py:190–192.

This blocks every endpoint — including GET /api/events/{session_id} (the SSE reconnect endpoint) and POST /api/chat/{session_id}. The non-holder Mongo change-stream slow path in _sse_response (routes/agent.py:799–861) is unreachable via HTTP for Worker-held sessions: _check_session_access raises 404 before _sse_response is ever called.

The unit tests in test_sse_holder_overlay.py call _sse_response directly with a pre-constructed AgentSession(holder_id="other:host:zzz") that is already in manager.sessions, so they bypass _check_session_access entirely and do not catch this gap.

Acceptance drill 2 states: "A Worker claims the lease within 30 s; the user reconnects to a fresh Main and sees the turn continuing." This cannot happen because Main returns 404 for any session whose lease is currently held by a Worker.

Fix direction: ensure_session_loaded (and/or _check_session_access) needs a read-only path for sessions where the lease is held by another process. When claim_lease fails because another holder owns an unexpired lease, the function could return a lightweight stub AgentSession with holder_id set to the current holder's ID (read from the Mongo doc) and broadcaster=None. _sse_response already branches correctly on is_holder and handles agent_session with a foreign holder_id via the Mongo change-stream path — the non-holder code path works as designed; the problem is only that it is never reached. Fix this →

P0 — _on_lease_lost requeues in-flight submissions for ALL held sessions, not just the one that lost its lease

_on_lease_lost(session_id) at session_manager.py:270 calls:

requeued = await store.requeue_claimed_for(self._holder_id)

requeue_claimed_for (session_persistence.py:707–710) runs:

await self.db.pending_submissions.update_many(
    {"status": "claimed", "claimed_by": holder_id},
    ...
)

This flips every claimed submission for this holder back to pending — across all sessions this process holds, not just session_id.

Concrete double-execution scenario: Main holds sessions A and B. B is mid-tool-call: claim_pending_submission has already set its submission to claimed. A's renew_lease returns None (either because another process took it, or because of a transient PyMongoError — see P1 below). _on_lease_lost(A) fires, calls requeue_claimed_for(Main._holder_id), and resets B's in-flight submission to pending. A Worker claims B's submission and starts executing it. Main's _drain_and_process finishes processing the same submission and calls mark_submission_done. Two processes have now executed the same submission against the same session context.

Fix direction: Pass session_id to requeue_claimed_for and filter on it: {"status": "claimed", "claimed_by": holder_id, "session_id": session_id}. Fix this →

P1 — renew_lease returns None on PyMongoError, which the heartbeat loop treats identically to "lease stolen"

session_persistence.py:614–617:

except PyMongoError as e:
    logger.warning(...)
    return None

session_manager.py:249–254:

renewed = await store.renew_lease(session_id, self._holder_id, ttl_s=30)
if renewed is None:
    # Lease lost — someone else holds it now.
    await self._on_lease_lost(session_id)

A transient Mongo network error fires _on_lease_lost, cancels the session's task, and (because of P0 #2) requeues in-flight submissions for all concurrently held sessions — on a temporary outage, not an actual lease theft.

Fix: renew_lease should raise (or return a sentinel distinct from None) on PyMongoError so the heartbeat loop can skip and retry rather than treating it as a lost lease. Fix this →

P1 — release_lease leaves holder_id intact; heartbeat can re-acquire a just-released lease

release_lease (session_persistence.py:624–630) sets lease.expires_at = now but does NOT clear lease.holder_id. After release_session_to_background releases the lease and pops the session from self.sessions (session_manager.py:332–333), the heartbeat loop may have already snapshotted session_ids (between lines 246–248) before the pop. The heartbeat then calls renew_lease(session_id, holder_id), whose filter matches {"_id": session_id, "lease.holder_id": holder_id} — the holder_id was not cleared. This re-extends the lease by 30 s, blocking the Worker from claiming the session for up to one full TTL after the intended handover. Fix this →

P1 — shutdown_session holds _lock for up to 5 seconds while awaiting task completion

session_manager.py:1446–1462 acquires self._lock and then calls both await self._store().release_lease(...) (external I/O) and await asyncio.wait_for(agent_session.task, timeout=5.0) inside the locked region. The lock is held for up to 5 seconds, serializing every other lock-requiring operation (heartbeat snapshot, release_session_to_background, grace-period sweep). Fix: release the lock before awaiting the task — move release_lease and wait_for outside async with self._lock. Fix this →

Overall: the core data-plane primitives (CAS lease, FIFO claim, change-stream tail, requeue) are well-designed and the test suite is thorough. The P0s are both in the boundary between the persistence layer and the session/route layer — they are fixable with targeted changes. The non-holder SSE path is fully implemented and just needs to be made reachable.

Claude finished @akseljoonas's task in 1m 47s —— View job

PR Review: Background sessions on Mongo control plane

Tasks

• Gather context (CLAUDE.md, REVIEW.md)
• Check P0 Compacting context #1: reconnect-to-Worker-held session (ensure_session_loaded)
• Check P0 Copying mcp tools #2: _on_lease_lost requeues all sessions (requeue_claimed_for scope)
• Check P1 fixes (renew_lease sentinel, release_lease holder_id, shutdown_session lock)
• Post findings

2 P0 still open (unchanged from prior review), 3 P1 not yet addressed
Verdict: changes requested

What I checked

• ensure_session_loaded (session_manager.py:828–878) — lease-claim path when another holder owns the session
• _on_lease_lost + requeue_claimed_for (session_manager.py:263–284, session_persistence.py:710–729) — scope of submission requeue
• renew_lease error handling (session_persistence.py:628–630) — PyMongoError sentinel
• release_lease update (session_persistence.py:638–641) — whether holder_id is cleared
• shutdown_session lock scope (session_manager.py:1478–1491) — I/O inside lock
• Heartbeat snapshot/iteration loop (session_manager.py:240–261) — no changes

P0 — Reconnect to Worker-held session returns 404 (unchanged)

ensure_session_loaded (session_manager.py:874–878) still returns None when claim_lease returns None:

if claimed is None:
    logger.info(
        f"Refusing restore of {session_id}: lease held by another process"
    )
    return None

_check_session_access in routes/agent.py raises HTTP 404 on None. The non-holder change-stream slow path in _sse_response remains unreachable via HTTP for any session currently held by a Worker. Acceptance drill 2 is still broken. Fix this →

P0 — _on_lease_lost requeues in-flight submissions for ALL held sessions (unchanged)

_on_lease_lost (session_manager.py:272) still calls:

requeued = await store.requeue_claimed_for(self._holder_id)

requeue_claimed_for (session_persistence.py:719–724) still filters only on {"status": "claimed", "claimed_by": holder_id} — no session_id predicate. A single lease loss triggers a broad requeue across every session this holder owns, enabling the double-execution scenario described in the prior review. Fix this →

The three P1s from the prior review (renew_lease PyMongoError sentinel, release_lease holder_id clear, shutdown_session lock scope) are also still open but not re-flagged here per re-review policy.

PR Review: Background sessions on Mongo control plane

Tasks

• Gather context (CLAUDE.md, REVIEW.md)
• Check P0 Compacting context #1: ensure_session_loaded reconnect path for Worker-held sessions
• Check P0 Copying mcp tools #2: _on_lease_lost / requeue_claimed_for scope
• Check P1 fixes (renew_lease sentinel, release_lease holder_id, shutdown_session lock)
• Post findings

View job run

Claude finished @akseljoonas's task in 5m 21s —— View job

Claude finished @akseljoonas's task —— View job

PR Review: Background sessions on Mongo control plane

Tasks

• Gather context (CLAUDE.md, REVIEW.md)
• Check P0 Compacting context #1: ensure_session_loaded reconnect path for Worker-held sessions
• Check P0 Copying mcp tools #2: _on_lease_lost / requeue_claimed_for scope
• Check P1 fixes (renew_lease sentinel, release_lease holder_id, shutdown_session lock)
• Post findings

1 P0
Verdict: changes requested

What I checked

• ensure_session_loaded stub construction (session_manager.py:892–933) — lease-claim failure path, stub fields, insertion guard
• _check_session_access + _enforce_gated_model_quota interaction (routes/agent.py:179–196, 139–157) — what stub flows trigger with session=None
• _sse_response is_holder branch (routes/agent.py:790–794) — whether stub reaches the non-holder change-stream path
• _on_lease_lost + requeue_claimed_for (session_manager.py:281–307, session_persistence.py:719–750) — session_id scope fix
• renew_lease PyMongoError propagation (session_persistence.py:615–633) + heartbeat catch (session_manager.py:265–270)
• release_lease holder_id clear (session_persistence.py:635–652)
• shutdown_session lock scope (session_manager.py:1521–1555)

Acknowledged fixes from prior review

All five prior findings are addressed:

• P0 Compacting context #1 (reconnect to Worker-held session): ensure_session_loaded now returns a stub AgentSession with holder_id=foreign_holder and broadcaster=None instead of None. _sse_response correctly takes the non-holder change-stream slow path because is_holder evaluates to False (broadcaster is None). SSE reconnect now works. ✓
• P0 Copying mcp tools #2 (_on_lease_lost requeues all sessions): _on_lease_lost now passes session_id=session_id to requeue_claimed_for (session_manager.py:292–293); requeue_claimed_for filters on that field when provided (session_persistence.py:737–738). Double-execution vector is closed. ✓
• P1 (renew_lease PyMongoError): renew_lease now propagates PyMongoError; the heartbeat loop catches it at session_manager.py:265 and skips with continue, never calling _on_lease_lost. ✓
• P1 (release_lease holder_id not cleared): release_lease now sets lease.holder_id: None alongside lease.expires_at: now (session_persistence.py:649), closing the heartbeat re-acquire window. ✓
• P1 (shutdown_session lock scope): Only the sessions.get lookup runs inside self._lock; release_lease and wait_for(task) run outside it (session_manager.py:1538–1553). ✓

P0 — Stub with session=None crashes _enforce_gated_model_quota on submit/chat endpoints

The stub returned by ensure_session_loaded for Worker-held sessions is constructed with session=None (session_manager.py:922). _enforce_gated_model_quota (routes/agent.py:156) unconditionally dereferences agent_session.session.config.model_name:

# routes/agent.py:154-156
if agent_session.claude_counted:
    return
model_name = agent_session.session.config.model_name  # AttributeError if session is None

The stub's claude_counted defaults to False (not read from the Mongo doc), so the early-return guard does not fire. Every invocation of _enforce_gated_model_quota against a stub raises AttributeError, which FastAPI surfaces as HTTP 500.

Affected paths (all call _check_session_access then _enforce_gated_model_quota):

• POST /api/submit (routes/agent.py:598–599) — user submitting text to an ongoing Worker turn
• POST /api/chat/{session_id} (routes/agent.py:641–654) — combined submit+stream
• POST /api/approve reaches _check_session_access but does not call _enforce_gated_model_quota, so it is not affected by this specific crash — but it would still return a stub that submit_approval would then handle correctly via the Mongo queue.

Scenario (post-drill-2): Worker holds session S. User reconnects from a fresh Main — SSE works. User then sends a new message: POST /api/submit → ensure_session_loaded returns stub (session=None) → _enforce_gated_model_quota crashes → HTTP 500.

Fix direction: In _enforce_gated_model_quota, add a guard for session=None — stubs represent sessions executing on a remote Worker and do not need local quota enforcement (the Worker already enforced quota when it built the real session). A one-line guard suffices:

if agent_session.session is None:
    return  # stub for remotely-held session; quota enforced by holder

Alternatively, read claude_counted from the loaded Mongo doc in the stub constructor so the guard at line 154 fires for already-charged sessions. Fix this →

Claude finished @akseljoonas's task in 1m 39s —— View job

PR Review: Background sessions on Mongo control plane

Tasks

• Gather context (CLAUDE.md, REVIEW.md)
• Check P0: _enforce_gated_model_quota crash with session=None stub
• Check for any new issues in the latest commit
• Post findings

1 P0
Verdict: changes requested

What I checked

• _enforce_gated_model_quota (routes/agent.py:139–170) — session null guard
• Stub AgentSession construction in ensure_session_loaded (session_manager.py:896–933) — session=None, claude_counted default
• POST /submit and POST /chat/{session_id} call sites (routes/agent.py:598–599, 653–654)
• Latest commit cd65715 (frontend-only: "Run-in-background button") — no new backend changes

Acknowledged fixes from prior review

The prior P0 finding from the last review round (the _enforce_gated_model_quota crash) is not yet addressed. Commit 07f0a85 ("Fix P0/P1 lease-handover correctness issues") touched session_persistence.py, session_manager.py, and tests — but not routes/agent.py.

P0 — _enforce_gated_model_quota crashes on stub session (unchanged from prior review)

ensure_session_loaded still returns a stub with session=None at session_manager.py:922. _enforce_gated_model_quota at routes/agent.py:156 unconditionally dereferences agent_session.session.config.model_name:

# routes/agent.py:154-156
if agent_session.claude_counted:
    return
model_name = agent_session.session.config.model_name  # AttributeError when session=None

The stub's claude_counted is not set in the constructor (session_manager.py:920–933), so it defaults to False (session_manager.py:112), and the early-return guard at line 154 does not fire.

Affected paths:

• POST /api/submit (routes/agent.py:598–599) — user sending text while a Worker holds the session
• POST /api/chat/{session_id} (routes/agent.py:653–654) — when text is not None and not approvals

Both raise AttributeError → HTTP 500 for any submit to a Worker-held session.

Fix: add a session=None guard at the top of _enforce_gated_model_quota, before line 154:

if agent_session.session is None:
    return  # stub for remotely-held session; quota enforced by holder

Alternatively, populate claude_counted=bool(meta.get("claude_counted")) in the stub constructor at session_manager.py:920–933 so already-charged sessions hit the existing guard. The one-line guard is the safer and more self-documenting fix. Fix this →